Skip to content

fix(check): exclude test-runner files from app compatibility scans#2595

Closed
NathanDrake2406 wants to merge 1 commit into
cloudflare:mainfrom
NathanDrake2406:nathan/fix-check-cjs-globals
Closed

fix(check): exclude test-runner files from app compatibility scans#2595
NathanDrake2406 wants to merge 1 commit into
cloudflare:mainfrom
NathanDrake2406:nathan/fix-check-cjs-globals

Conversation

@NathanDrake2406

@NathanDrake2406 NathanDrake2406 commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Overview

Item Detail
Goal Keep test-runner files out of application compatibility results.
Root cause vinext check recursively treated every JavaScript and TypeScript file as possible app runtime source.
Core change Use an app-compatibility candidate set that excludes test/spec modules and conventional Jest, Playwright, and Vitest config files.
Expected impact Test-only imports and CommonJS globals no longer appear as app migration blockers.

Why

vinext check should report compatibility for files that can contribute to the application bundle. Test modules and test-runner configuration execute under their own runners, so treating them as vinext runtime source creates migration work that does not exist.

This was reproduced in a real Next.js to vinext migration of the movies-ranking app. The report marked CommonJS globals unsupported in exactly these files:

✗  __dirname / __filename (CommonJS globals) — CJS globals unavailable in ESM — use fileURLToPath(import.meta.url) / dirname(...), or import.meta.dirname / import.meta.filename (Node 22+)
   src/app/mobile-layout-alignment.test.ts
   vitest.config.ts

Both files are outside the vinext application runtime. The same checkout completed vinext build, and its targeted Vitest file passed. With this PR's CLI, the app reports:

Overall: 98% compatible (20 supported, 1 partial, 0 issues)
Area Invariant What this PR changes
Compatibility source discovery Only app-runtime candidates should affect app compatibility. Excludes *.test.*, *.spec.*, and conventional Jest, Playwright, and Vitest config files.
Runtime modules Ordinary shared modules must remain visible even when their names contain config. Keeps root site.config.ts and other non-test-runner configs in both scans.
Import checks Test-only next/* imports do not constrain the deployed app. Applies the same candidate boundary to import support.
Convention checks Test-only CJS globals and ViewTransition imports do not constrain the deployed app. Applies the same candidate boundary to per-file convention checks.

What changed

Scenario Before After
__dirname in a Vitest test/config Reported as an unsupported app migration issue. Excluded from application compatibility scanning.
Unsupported next/* import used only by tests Counted against app compatibility. Excluded from application compatibility scanning.
__dirname or unsupported import in site.config.ts Reported. Still reported.
Validation
  • vp test run tests/check.test.ts: 145 tests passed.
  • vp check: all 2,430 files formatted; no warnings, lint errors, or type errors across 1,080 files.
  • vp run vinext#build: package build passed.
  • Patched vinext check against movies-ranking/.worktrees/nathan-vinext-jsonc-install: 98% compatible, 0 issues.
  • bun run build:vinext in that movies-ranking worktree: all five build environments passed.
  • bunx vitest run src/app/mobile-layout-alignment.test.ts: 4 tests passed.
  • Pre-commit full checks, focused tests, and knip passed.
Risk / compatibility
  • No runtime, build-output, or public API changes.
  • Scanner exclusion is basename-based and anchored.
  • Only test/spec modules and known test-runner config names are excluded.
  • Ordinary runtime config modules remain covered by explicit regressions.
Non-goals
  • Changing support for CommonJS globals in application runtime modules.
  • Inferring the complete Vite module graph during the lightweight compatibility check.
  • Excluding every file whose name contains config.

References

Reference Why it matters
#1740 Existing runtime support for CJS path globals in eligible server modules; not the failing boundary here.
#1650 Older middleware/proxy runtime work; also distinct from this scanner false positive.

@NathanDrake2406
fix(check): classify CJS path globals as partially supported
318944e 
vinext check reports every __dirname and __filename occurrence as an unsupported migration issue, even though eligible server modules receive those globals automatically. This makes compatible server code look like a required rewrite during migration.

The checker cannot determine whether every scanned source file is server-only or client-reachable. Classify the feature as partial, state the server and client boundary explicitly, and cover the original missing type:module combination in the formatted report.
@pkg-pr-new

pkg-pr-new Bot commented Jul 11, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2595

npm i https://pkg.pr.new/create-vinext-app@2595

npm i https://pkg.pr.new/vinext@2595

commit: 318944e

@NathanDrake2406

NathanDrake2406 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Closing because this patch only reclassified the compatibility report. The requested fix is to remove the underlying migration incompatibility at the affected runtime or init boundary; I am returning to the concrete app reproduction before proposing a replacement.

@NathanDrake2406 NathanDrake2406 changed the title fix(check): classify CJS path globals as partially supported fix(check): exclude test-runner files from app compatibility scans Jul 11, 2026
@NathanDrake2406

NathanDrake2406 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

GitHub would not reopen this PR after the requested force-push because the fork branch was recreated. The corrected root-cause patch is now #2596.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@NathanDrake2406